-
Notifications
You must be signed in to change notification settings - Fork 694
Speedup GetSpellingSuggestion #1640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This should only matter in erroring code. Are you silencing a whole bunch of errors? |
I'm not sure? The output above shows the only 2 errors that are reported. |
If possible I'd print something out each time he function is called and count how often it is, or look around for ts-ignores/ts-expect-error. I think we can totally accept an optimization like this, but we certainly aren't expecting the diagnostic code paths to be hot. |
There are around 3.1K ts-expect-error but I see GetSpellingSuggestion called around 750K times. I checked how many options were considered for each call (i.e. how many calls we executed to compute levenshtein distance, not just how many were provided):
A majority of calls only execute up to 2-3 computations, but for some reason there's 122K calls that consider exactly 175 possibilities...
|
That many ignored errors is terrifying; I would strongly suggest converting those to casts or similar. But, would be good to improve nevertheless... |
Can you characterize these for us? Why so many? We only compute spelling suggestions we can't find an identifier or property with that name -- situations that should be very, very uncommon in theory (and usually have straightforward type-side fixes available, e.g. |
func levenshteinWithMax(s1 []rune, s2 []rune, maxValue float64) float64 { | ||
previous := make([]float64, len(s2)+1) | ||
current := make([]float64, len(s2)+1) | ||
func ensureSize(slice []float64, desired int) []float64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just use slices.Grow
} | ||
|
||
buffers.previous = ensureSize(buffers.previous, len(s2)+1) | ||
previous := buffers.previous |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would honestly benefit from simply using a sync.Pool
to store the buffers globally, rather than scoping the object. Means less plumbing and callers don't have to know anything about it.
Noticed we weren't using buffers while trying the Kibana repro.
Before:
After: